The assumption is that the index only iterates over business days for which there are around 252 in year (by convention)

The assumption is that the index only iterates over business days for which there are around 252 in year (by convention)
years = (returns.index[-1] - returns.index[0]).days/252

if we use this assumption then period between 01/07/2005 -- 22/05/2023 will be approx 26 years. Hence this is essentially overestimating the years.

Hence this is essentially overestimating the years.

yes, this is exactly what is happening here.

num_days = returns.index[-1] - returns.index[0]).days the cagr function calculates the number of days like this. then does num_days/period to get years the problem is that using 252 gives the wrong number of years between 2 given dates. Using 365 fixes it.

the pull request is actually proposing to remove the periods parameter completely. the reason behind is that any value other than 365 will give the wrong number of years. And the wrong number of years impacts the CAGR. As the "A" in CARG stands for annual, then there is no need to have this as a parameter. Or at least, I can't think of a case where anything other than 365 would work. Unless the way of calculating num_days changes

Unless the way of calculating num_days changes

Yes
".days" attribute does not consider trading days but calculates the actual number of days between two dates.

@gnzsnz, the period parameter shouldn't be removed because it will allow users to evaluate portfolios based on the time frame that's being observed. The value that is needed for the actual calculation is years = len(returns)/periods or
res = abs(total + 1) ** (periods/len(returns)) - 1.
The idea is based on the definition of CAGR discussed in the article linked below. This will allow users to calculate CAGR for any time period.

An alternative approach is to use log returns, but it's best to stick with normal returns because quantstats uses normal returns for most of its calculations. I compared log returns and normal CAGR approach on daily period, and I got approximately the same result. Using the CAGR function currently implemented in quantstats gives a different result.

If you could review your code and implement to version of CAGR discussed in the article, that would be great.

https://www.investopedia.com/terms/a/annualized-total-return.asp#:~:text=The%20main%20difference%20between%20them,two%20measures%20are%20the%20same.

@tiloye thanks for your feedback. you really got me thinking. i did quite some reading to understand this better, but i have to say that I still think that we should remove the periods parameter. I think is the best alternative, and I've just realized that I got it right in the first place just by chance.

The thing is that Annualized Total Return and the Compound Annual Growth Rate (CAGR) are essentially the same. Now what really got me thinking was your point stating that we should be able to evaluate CAGR for any time period. And I realized that the periods parameter is not going to achieve that.

Basically we need to calculate the number of years for the returns time series (returns parameter) and we could do that in two different ways, one would be to use the current logic and get the actual number of days between start and end date. Another option is what you propose to get the length of the time series, or number of periods.

If we go for actual number of days as it's today, we need to use 365 as the denominator. if we use length we need use 252. But that's it it could be 365 or 252, and it should be aligned with the method used to calculate the number of days.

But in any case having a parameter for periods (which is basically our denominator) can only cause problems. Because anything other than the default will return a wrong number.

if we want to have the option to calculate returns over different periods, then we need to provide different periods in the returns series, not change the periods parameter.

Hope my point is clear. As I said before, I did not realize this until I really went into it.

@gnzsnz, I get your point. If we are going to use the difference between the dates, then 365 should be the denominator. I also confirmed it holds for return series that isn't up to a year. Thanks for pointing that out.

@ranaroussi are you OK with this PR?

This looks good to me, does this also take care of some other possible metrics miscalculations introduced with the period parameter changes? If everything else looks properly calculated with this PR, please merge asap

@ranaroussi kind reminder

is this project abandoned? tons of unsolved PRs

#323 is still open

Since this library no longer seems to be maintained, we have created a duplicate of it that we plan to maintain and improve over time. If you'd like to use our version check it out here: https://github.com/Lumiwealth/quantstats_lumi

Please feel free to fork and send your pull request to this new library, we will be actively monitoring the library and approving PRs

Since this library no longer seems to be maintained, we have created a duplicate of it that we plan to maintain and improve over time. If you'd like to use our version check it out here: https://github.com/Lumiwealth/quanstats_lumi

Please feel free to fork and send your pull request to this new library, we will be actively monitoring the library and approving PRs

Cool library, Nice! I will surely take a look at it.

I don't have a good solution to this problem, but just wanted to note that setting periods=365 will only make results more accurate, but will not fix the problem entirely.

This calculation:
years = (returns.index[-1] - returns.index[0]).days / periods
wrongly assumes that the first and the last day is a trading day.

If these are not trading days, doing returns.index[-1] - returns.index[0] will always result in a number less than 365 and the whole cagr calculation will be a slight overestimation.

For example, lets choose a period 2020-01-01 to 2020-12-31.
2020-01-01 was a non trading day, so when _yf downloads returns, the Series will start at 2020-01-02.
Doing (returns.index[-1] - returns.index[0]).days will be equal to 364

Since this library no longer seems to be maintained, we have created a duplicate of it that we plan to maintain and improve over time. If you'd like to use our version check it out here: https://github.com/Lumiwealth/quantstats_lumi

Please feel free to fork and send your pull request to this new library, we will be actively monitoring the library and approving PRs

@grzesir have you considered PEP 541, you can keep pypi package name if you apply for it. I understand that you need to create an issue under pypi support and tag it as PEP 541, here is an example pypi/support#3932